Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bugfix: Bring back singleton option when creating a new Session Bean and interfaceless EJBs #6257

Merged
merged 1 commit into from
Jul 31, 2023
Merged

Bugfix: Bring back singleton option when creating a new Session Bean and interfaceless EJBs #6257

merged 1 commit into from
Jul 31, 2023

Conversation

matthiasblaesing
Copy link
Contributor

The options shown when selecting New > Session Bean are based on the capabilities of the used enterprise project.

The check missed some Jakarta EE versions and so were disabled.

Analysis done by @asbachb

Closes: #4892
Closes: #4830

@matthiasblaesing matthiasblaesing added Java EE/Jakarta EE [ci] enable enterprise job enterprise [ci] enable enterprise job labels Jul 25, 2023
@matthiasblaesing matthiasblaesing added this to the NB19 milestone Jul 25, 2023
@matthiasblaesing
Bugfix: Bring back singleton option when creating a new Session Bean …
18e2961 
…and interfaceless EJBs

The options shown when selecting New > Session Bean are based on the
capabilities of the used enterprise project.

The check missed some Jakarta EE versions and so were disabled.

Analysis done by @asbachb

Closes: apache#4892
Closes: apache#4830
@matthiasblaesing matthiasblaesing force-pushed the session_bean_interface_less_ejb branch from fe6d2b4 to 18e2961 Compare July 25, 2023 20:18
@matthiasblaesing
Copy link
Contributor Author

@asbachb I agree with your analysis in #6212, meaning, that there are version checks missing. I consider this the less invasive approach though, as it only affects only modified code sites.

To identify locations to look at I used this procedure:

  • identify potentially affected modules: grep -lr "isEjb3.*Supported" * | cut -f 1 --delimiter "/" | sort -u
  • open all potentially affected modules and use the "Find Usages" search on the isEjb3* methods
  • review the calls

The low hanging fruits were fixed, other locations need some more changes for example to annotation scanning. These classes were marked @todo: Support JakartaEE.

@matthiasblaesing matthiasblaesing requested a review from asbachb July 25, 2023 20:21
@asbachb
Copy link
Collaborator

asbachb commented Jul 25, 2023

I'm still not convinced this is the right approach as it keeps the need to touch the code on every new EJB release.

asbachb
asbachb reviewed Jul 25, 2023
View reviewed changes
....ejbcore/src/org/netbeans/modules/j2ee/ejbcore/api/methodcontroller/EjbMethodController.java
@@ -72,7 +72,7 @@ public EjbType run(EjbJarMetadata metadata) throws Exception {
Project project = FileOwnerQuery.getOwner(ejbClassFO);
if (project != null){
J2eeProjectCapabilities projectCap = J2eeProjectCapabilities.forProject(project);
allowsNoInterface = projectCap != null ? projectCap.isEjb31LiteSupported() : false;
allowsNoInterface = (projectCap != null && (projectCap.isEjb31LiteSupported() || projectCap.isEjb40LiteSupported()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's about 3.2 lite? From my understanding EJB lite is a subset of EJB full. So if EJB lite should support it as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reading of isEjb31LiteSupported is, that it is true if EJB 3.2 is supported. Assuming EJB follows the convention, that breaking changes are communicated by a change in the major number, this makes sense (repeating myself: I assume 3.2 is a superset of 3.1).
Yes if isEjb31Supported returns true, isEjb31LiteSupported will, the same is true for the other EJB capability checks.

@matthiasblaesing
Copy link
Contributor Author

I'm still not convinced this is the right approach as it keeps the need to touch the code on every new EJB release.

Maybe. If the next EJB release is breaking (aka 5.X), then yes, but then you need to check that code anyway to see if it affected by the breakage. If it is 4.1 (a true superset), then

netbeans/enterprise/j2ee.common/src/org/netbeans/modules/j2ee/common/J2eeProjectCapabilities.java

Lines 158 to 162 in b79fd30

public boolean isEjb40Supported() {
J2eeModule.Type moduleType = provider.getJ2eeModule().getType();
boolean ee9 = ejbJarProfile != null && (ejbJarProfile.equals(Profile.JAKARTA_EE_9_FULL) || ejbJarProfile.equals(Profile.JAKARTA_EE_9_1_FULL) || ejbJarProfile.equals(Profile.JAKARTA_EE_10_FULL));
return ee9 && (J2eeModule.Type.EJB.equals(moduleType) || J2eeModule.Type.WAR.equals(moduleType));
}

and

netbeans/enterprise/j2ee.common/src/org/netbeans/modules/j2ee/common/J2eeProjectCapabilities.java

Lines 171 to 175 in b79fd30

public boolean isEjb40LiteSupported() {
J2eeModule.Type moduleType = provider.getJ2eeModule().getType();
boolean ee9Web = ejbJarProfile != null && (ejbJarProfile.equals(Profile.JAKARTA_EE_9_WEB) || ejbJarProfile.equals(Profile.JAKARTA_EE_9_1_WEB) || ejbJarProfile.equals(Profile.JAKARTA_EE_10_WEB));
return isEjb40Supported() || (J2eeModule.Type.WAR.equals(moduleType) && ee9Web);
}

can centrally be updated to cover the next JakartaEE version and you are done.

API breaks always mean, that you have to check code, especially if the compiler can't really help you to diagnose problems. Nothing special there. The classes I marked as @todo show IMHO that we are currently seeing only the tip of the iceberg and there is more breakage lingering.

@asbachb
Copy link
Collaborator

asbachb commented Jul 25, 2023

Condensed it's we surely break everything EJB related with a new version vs. we most likely won't break anything as the new ejb spec is feature par with it's predecessor.

So basically every new EJB version will for sure create bugs if not adjusted on time and if not adjusted on every if statement.

Is there any EJB release which removed/limited functionality of the prior EJB releases?

@neilcsmith-net
Copy link
Member

I didn't merge this for 19-rc3 as it seems discussion still ongoing. Adding do-not-merge label. Please remove that label (or close the PR) depending on decision for NB19.

@asbachb please keep in mind this is for delivery into NB19. It doesn't preclude a "better" approach in a future release. Please review based on whether it addresses some immediate issues for the next release.

Thanks both!

@neilcsmith-net neilcsmith-net added the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Jul 27, 2023
@asbachb asbachb removed the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Jul 30, 2023
@neilcsmith-net neilcsmith-net merged commit a9904d9 into apache:delivery Jul 31, 2023
@matthiasblaesing matthiasblaesing deleted the session_bean_interface_less_ejb branch August 12, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asbachb asbachb asbachb approved these changes

Assignees
No one assigned
Labels
enterprise [ci] enable enterprise job Java EE/Jakarta EE [ci] enable enterprise job
Projects
NetBeans JavaEE related
Status: Done
Milestone
NB19
Development

Successfully merging this pull request may close these issues.
3 participants
@matthiasblaesing @asbachb @neilcsmith-net